Skip to content

Add GIN expression index on boxel_index.types so type-anchored search filters are index-served#5533

Open
FadhlanR wants to merge 2 commits into
mainfrom
cs-12082-add-a-gin-index-on-boxel_indextypes-so-type-anchored-search
Open

Add GIN expression index on boxel_index.types so type-anchored search filters are index-served#5533
FadhlanR wants to merge 2 commits into
mainfrom
cs-12082-add-a-gin-index-on-boxel_indextypes-so-type-anchored-search

Conversation

@FadhlanR

Copy link
Copy Markdown
Contributor

Summary

Type-anchored filters — the hottest filter shape in the system — compile to a per-row containment predicate (types-contains in packages/runtime-common/expression.ts):

COALESCE(i.types, '[]'::jsonb) @> '["<type>"]'::jsonb

Postgres only serves a predicate from an index whose expression matches the query's expression. Both tables carry a GIN index on the bare types column, which can never match the COALESCE(...) form — so every type-anchored search seq-scans. Since typesContains is the only code path that filters on types (all other references are SELECT columns), those bare-column indexes serve no query at all.

This migration replaces them with expression indexes matching the emitted predicate exactly, using jsonb_path_ops (smaller/faster than the default jsonb_ops; supports @>, the only operator this predicate uses):

CREATE INDEX CONCURRENTLY ... USING GIN ((COALESCE(types, '[]'::jsonb)) jsonb_path_ops);

on both boxel_index and boxel_index_working (the search path can target either via useWorkInProgressIndex). CONCURRENTLY avoids locking writes during the build in production. The query engine is unchanged — the COALESCE wrapper is deliberate (NULL types must be a definite FALSE so an enclosing NOT (...) keeps never-indexed rows). SQLite (client-side index) is unaffected; the schema converter emits no indexes.

The host SQLite schema file is regenerated (pnpm make-schema) so its filename timestamp matches the latest migration; content is unchanged.

Verification

Against a locally indexed dataset (3.6k rows), the planner picks the new index unaided:

Bitmap Heap Scan on boxel_index i  (actual time=0.085..0.572 rows=820 loops=1)
  Recheck Cond: (COALESCE(types, '[]'::jsonb) @> '["@cardstack/base/ts-file-def/TsFileDef"]'::jsonb)
  ->  Bitmap Index Scan on boxel_index_types_containment_idx  (actual time=0.057..0.057 rows=820 loops=1)
        Index Cond: (COALESCE(types, '[]'::jsonb) @> '["@cardstack/base/ts-file-def/TsFileDef"]'::jsonb)
  • up → down → up runs clean; down restores the bare-column indexes under their original auto-generated names so the older migrations' downs still find them.
  • pnpm lint in packages/postgres passes (lint:js, lint:migrations, lint:types).

Follow-up after deploy: measure before/after SQL time on staging via the realm:search-timing log channel.

Fixes CS-12082

🤖 Generated with Claude Code

Type-anchored search filters compile to
COALESCE(types, '[]'::jsonb) @> '["<type>"]'::jsonb, which the planner
can only serve from an index on that exact expression. The existing
bare-column GIN indexes on types never matched it (and nothing else
filters on types), so type filters seq-scanned. Replace them with
jsonb_path_ops expression indexes on both boxel_index and
boxel_index_working.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Preview deployments

Host Test Results

    1 files      1 suites   3h 0m 46s ⏱️
3 547 tests 3 532 ✅ 15 💤 0 ❌
3 566 runs  3 551 ✅ 15 💤 0 ❌

Results for commit 127e57b.

Realm Server Test Results

    1 files  ±0      1 suites  ±0   14m 12s ⏱️ - 3m 17s
1 875 tests ±0  1 875 ✅ ±0  0 💤 ±0  0 ❌ ±0 
1 954 runs  ±0  1 954 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 127e57b. ± Comparison against earlier commit b3001bf.

An interrupted CREATE INDEX CONCURRENTLY leaves an INVALID index that
the planner ignores, and IF NOT EXISTS would skip it on the retry —
the migration would then record success while delivering nothing. Drop
the target name unconditionally before each build instead (safe: the
migration only re-runs when a prior attempt failed to record), in both
up() and the down() restores.

Also note the expected node-pg-migrate break-single-transaction warning
in the migration header, and cross-reference the index from the
types-contains SQL in expression.ts, whose compiled expression must
stay verbatim-identical to the index expression.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@FadhlanR
FadhlanR marked this pull request as ready for review July 17, 2026 09:10
@FadhlanR
FadhlanR requested review from a team and habdelra July 17, 2026 09:10

@habdelra habdelra left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Reviewed with a focus on whether the planner can actually harness this index given the shape of the queries the engine emits — an expression index is only useful if the emitted predicate reaches the scan node intact, and past experience with the GIN search_doc index shows the query side sometimes has to change shape before an index becomes usable.

Bottom line: the index is harnessed as-is; no query-side change is needed, and I found no blocking issues. I verified this empirically against a real indexed dataset by running the full emitted _search query shape (OR of equivalent type-key spellings, prerendered_html LEFT JOIN, jsonb_tree lateral joins, GROUP BY/ORDER BY/LIMIT, and generic plans with bind parameters) — plans and analysis are in the inline comment on the CREATE INDEX. The distinction from the search_doc case: there the predicate itself (->> extraction) was fundamentally not GIN-servable and had to be rewritten into @> containment; here the predicate is already containment-shaped, and the mismatch was purely on the index side — the bare-column index expression could never match the COALESCE(...) the query emits. This migration moves the index expression to match, which is the correct half to change.

Also verified:

  • Nothing else filters on typestypesContains call sites in index-query-engine.ts are the only WHERE-clause references repo-wide; every other reference is a SELECT column. Dropping the bare-column indexes removes dead weight only (confirmed in-plan: they never appear even in the pre-migration state).
  • Migration mechanicsnoTransaction() + CONCURRENTLY + unconditional-DROP-before-CREATE matches the established pattern for GIN index migrations in this directory (the markdown-FTS swap helper handles interrupted INVALID builds the same way). down() faithfully restores the original auto-generated names and opclass (inline comment).
  • Schema file changes are required, not churn — host dev/test builds hard-fail when the schema filename lags the latest migration, and make-schema prunes prior files (inline comment on the deleted file).

Two non-blocking items:

  1. PR description: drop the Fixes CS-12082 line. This repo is public and the tracker is private — tracker IDs don't go in GitHub-facing prose; the linkage belongs on the Linear side by attaching this PR's URL to the ticket.
  2. Optional hardening: the coupling between the emitted SQL and the index expression is currently guarded only by comments on each side. A realm-server test (that suite runs against real Postgres) could EXPLAIN a type-filtered search and assert the plan references boxel_index_types_containment_idx — turning the contract into an executable check. Fine as a follow-up or not at all, but it would catch the silent regression (correct results, seq-scan speed) that a wrapper edit would cause.

Adjacent and out of scope, noting for whoever touches it next: the worked example in the handleFieldArity doc comment (index-query-engine.ts, ~line 1540) illustrates type conditions as a jsonb_array_elements_text(types) cross join, which is not the shape they compile to — the example predates the membership-predicate form and could be tidied opportunistically.

Comment on lines +35 to +38
CREATE INDEX CONCURRENTLY boxel_index_types_containment_idx
ON boxel_index
USING GIN ((COALESCE(types, '[]'::jsonb)) jsonb_path_ops);
`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] The open question for an expression index like this is whether the planner still reaches it inside the full query the engine emits — an isolated EXPLAIN on the bare predicate doesn't prove that. Some context on why that's a real concern here, and the verification result:

What the engine actually emits. A type filter never appears as a single containment predicate. typeCondition in packages/runtime-common/index-query-engine.ts expands one card ref into an OR of containment arms — one per equivalent spelling of the type key (internalKeysFor: RRI form, real-URL form, virtual-alias form) — and _search ANDs that with i.realm_url = $n, the is_deleted check, i.type = $n, and a not-errored predicate that references the LEFT-JOINed prerendered_html row. Plural-path field filters additionally splice CROSS JOIN LATERAL jsonb_tree(...) into the FROM clause, and everything runs under GROUP BY i.url, i.type + ORDER BY ... LIMIT. Any of those wrappers could in principle keep the qual from being pushed into the scan of i.

Verified against a real indexed dataset (13.5k rows / 23 realms, Postgres 16.3), running the full emitted _search shape rather than the bare predicate. The planner pushes the whole OR into the scan as a BitmapOr over this index, and BitmapAnds it with the realm/type btree:

Bitmap Heap Scan on boxel_index i  (actual rows=659)
  Recheck Cond: ((COALESCE(types,'[]'::jsonb) @> '["<url form>"]' OR
                  COALESCE(types,'[]'::jsonb) @> '["<rri form>"]')
                 AND realm_url = $realm AND type = 'instance')
  ->  BitmapAnd
        ->  BitmapOr
              ->  Bitmap Index Scan on boxel_index_types_containment_idx (rows=659)
              ->  Bitmap Index Scan on boxel_index_types_containment_idx (rows=0)
        ->  Bitmap Index Scan on boxel_index_realm_url_type_index (rows=1186)

The same holds in the three variations that could have defeated it:

  • Generic plan with bind parameters (EXPLAIN (GENERIC_PLAN) with $n placeholders — the extended-protocol worst case, relevant because node-postgres sends parameterized statements): index cond binds COALESCE(types, '[]'::jsonb) @> $3 directly.
  • With a jsonb_tree lateral join present (a plural-path eq alongside the type filter): the containment qual still lands in the scan of i, before the lateral fan-out.
  • The hasFileType / hasInstanceType SELECT 1 ... LIMIT 1 shape: index-served, sub-ms.

And the before-state confirms the premise for dropping the bare-column indexes: with only boxel_index_types_index present, the planner scans via the realm/type btree and evaluates the containment row-by-row as a post-scan filter (Rows Removed by Filter: 527) — the bare GIN never appears in any plan.

One boundary worth knowing: the index only helps where the containment sits in an AND chain over i columns. A type condition nested under a filter-level any: whose sibling branch references a json_tree alias becomes an OR spanning the lateral join, which no index can serve — that's inherent to OR-across-a-join, and the dominant shapes (the on type anchor and card-type filters, which are always AND-ed) are the ones served.

For background on why no query-side change is needed here (unlike the singular string eq filter, which had to be rewritten from search_doc -> 'a' ->> 'b' = $n extraction into the JsonContains search_doc @> '{...}'::jsonb containment node to become GIN-servable): GIN jsonb opclasses serve @>, and field-extraction predicates are not GIN-servable at all. The types predicate is already containment-shaped — the only mismatch was on the index side, which this migration fixes.

Comment on lines +6 to +7
// jsonb_path_ops is smaller and faster than the default jsonb_ops and
// supports @>, the only operator this predicate uses.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] jsonb_path_ops is the right call for this predicate, and it's worth spelling out the two capabilities it trades away so a future change on this column knows when the choice stops fitting:

  1. It serves only @>. The key-exists operators (?, ?|, ?&) and everything else in the default jsonb_ops repertoire are unsupported. The types-contains predicate is pure containment, so nothing is lost — but a future filter wanting e.g. "does any type key start with…" or key-existence semantics would need a jsonb_ops index (or a different structure entirely).
  2. It produces no index entries for empty arrays/objects. Rows whose types is NULL index as '[]' under the COALESCE and therefore have no entries — which is exactly right: they can never satisfy a containment test against a non-empty right-hand side, and the engine always binds a one-element array ('["<key>"]'), never an empty one. The degenerate @> '[]' query (true for every row, answerable only by a full index scan) is not a shape the engine can emit.

Nothing to change — this is confirmation that both trade-offs are safe for the emitted predicate, plus the conditions under which they'd stop being safe.

Comment on lines +59 to +61
// Restore under the original auto-generated names so the down migrations
// of 1735668047598 and 1735832183444 still find them. Same
// drop-then-create pattern as up() to clear INVALID leftovers on retry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Verified the restore fidelity: the referenced migrations create these via pgm.createIndex(table, 'types', { method: 'gin' }), whose auto-generated names are exactly boxel_index_types_index / boxel_index_working_types_index with the default jsonb_ops opclass on the bare column — which is what's recreated here, so their down()s (pgm.dropIndex(table, 'types')) resolve by name. 👍

One wording nit while you're in here: "still find them" is relative-time phrasing — the comment reads cleaner as a timeless contract, e.g. "…so the down migrations of 1735668047598 and 1735832183444 find them by name." (Referencing migrations by timestamp is fine — they're immutable filenames in this directory, unlike tracker or PR numbers, which don't belong in comments.)

Comment on lines +557 to +561
// The boxel_index_types_containment_idx GIN indexes (migration
// 1784272066344) cover this exact expression — Postgres only uses an
// expression index when the query expression matches it verbatim, so
// changing the SQL here (e.g. the COALESCE wrapper) un-indexes type
// filters unless the index expression moves with it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] Good — this comment is the guard that keeps the two sides of the contract from drifting, since nothing executable enforces the coupling. One clarification worth folding in: "matches it verbatim" slightly overstates the strictness, and knowing where the real line sits helps a future editor judge what's safe.

The planner matches on the parsed expression tree, not the SQL text. So these are all fine and don't break index use:

  • the alias qualification (i.types here vs. bare types in the index DDL),
  • whitespace, keyword case, COALESCE vs coalesce,
  • the right-hand side of @> (it's the query value — only the left operand must match the indexed expression).

What does break it is any semantic change to the left operand's tree: a different empty-array placeholder ('{}'::jsonb), an added cast, or restructuring into types IS NOT NULL AND types @> …. The failure mode is silent — queries stay correct, they just fall back to evaluating containment row-by-row behind the realm/type btree.

Suggested tweak: "…Postgres only uses an expression index when the query's left operand structurally matches the indexed expression (aliasing and whitespace don't matter; any semantic change to the COALESCE wrapper does), so changing the SQL here un-indexes type filters unless the index expression moves with it."

@@ -1,235 +0,0 @@
-- This is auto-generated by packages/realm-server/scripts/convert-to-sqlite.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Code 🤖] For anyone wondering whether deleting this file (rather than just adding the renamed one) is correct: yes, on two grounds.

  1. pnpm make-schema (packages/postgres/scripts/schema-dump.sh) starts by removing every *_schema.sql in this directory before writing the fresh dump — the generator's contract is exactly one file, so two sitting side-by-side on the base branch is leftover state that this regeneration converges.
  2. The rename itself is load-bearing, not churn: getLatestSchemaFile() in packages/host/config/environment.js throws in dev/test builds when the newest schema file's timestamp prefix doesn't match the newest migration's — so every new migration must ship with a regenerated schema file, even an index-only one.

The content coming out unchanged (100%-similarity rename) is expected: the SQLite converter emits tables only, no indexes. The client-side SQLite path evaluates the types predicate as a json_each EXISTS with no index either way, so this PR's behavior change is Postgres-only by construction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants